Add support for EKS Pod Identity #1781
Conversation
kevinrudde
force-pushed
the
add-support-for-pod-identity
branch
4 times, most recently
from
0e0a48b to
6d2adb6
Compare
| * | ||
| * @see https://docs.aws.amazon.com/eks/latest/userguide/pod-identities.html | ||
| */ | ||
| final class PodIdentityProvider implements CredentialProvider |
There was a problem hiding this comment.
What's about reusing the existing ContainerProvider instead?
It looks like this is what the official AWS SDK does: https://github.com/aws/aws-sdk-php/blob/master/src/Credentials/EcsCredentialProvider.php
In that way, we could leverage the AuthToken in both full / relative URI
Endpoint is one of:
http://169.254.170.2.AWS_CONTAINER_CREDENTIALS_RELATIVE_URI- pr
AWS_CONTAINER_CREDENTIALS_FULL_URI
Auth Token is one of:
AWS_CONTAINER_AUTHORIZATION_TOKEN- content of
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE - nothing
There was a problem hiding this comment.
I changed my implementation to reuse the ContainerProvider like the AWS SDK does 😄
|
|
||
| use AsyncAws\Core\Exception\RuntimeException; | ||
|
|
||
| final class TokenFileLoader |
There was a problem hiding this comment.
I think this class could be a trait.
| new ContainerProvider($httpClient, $logger), | ||
| new PodIdentityProvider($httpClient, $logger), | ||
| new InstanceProvider($httpClient, $logger), |
There was a problem hiding this comment.
not related to your PR, but I discovered that AWS uses either EscProvider or InstanceProvider in the default chain:
if (self::shouldUseEcs()) {
$defaultChain['ecs'] = self::ecsCredentials($config);
} else {
$defaultChain['instance'] = self::instanceProfile($config);
}
I think we could safely do the same 🤔
|
|
||
| // fetch credentials from pod identity agent endpoint | ||
| try { | ||
| $response = $this->httpClient->request('GET', $fullUri, ['headers' => ['Authorization' => $tokenFileContent]]); |
There was a problem hiding this comment.
the official AWS SDK checks if the URL is valid, I believe we should do the same:
kevinrudde
force-pushed
the
add-support-for-pod-identity
branch
3 times, most recently
from
3e1cce1 to
18c06e5
Compare
| } | ||
|
|
||
| $tokenFile = $configuration->get(Configuration::OPTION_POD_IDENTITY_AUTHORIZATION_TOKEN_FILE); | ||
| if (false === empty($tokenFile)) { |
There was a problem hiding this comment.
| if (false === empty($tokenFile)) { | |
| if (!empty($tokenFile)) { |
| } | ||
| } | ||
|
|
||
| if (!$this->isUriValid($fullUri)) { |
There was a problem hiding this comment.
can be move earlier (at line 60 for instance)
| private function getFullUri(Configuration $configuration): ?string | ||
| { | ||
| $relativeUri = $configuration->get(Configuration::OPTION_CONTAINER_CREDENTIALS_RELATIVE_URI); | ||
| $fullUri = $configuration->get(Configuration::OPTION_POD_IDENTITY_CREDENTIALS_FULL_URI); |
There was a problem hiding this comment.
can be moved at line 144 (no need to instanciate the fullUri variable when the relativeUri is not null
| * | ||
| * @return bool true if the IP is a loopback address, false otherwise | ||
| */ | ||
| public static function isLoopBackAddress(string $host) |
There was a problem hiding this comment.
no need to be static, nor to be public.
lets avoid exposing ( = maintaining) new methods
| if ('https' !== $parsedUri['scheme']) { | ||
| $host = trim($parsedUri['host'] ?? '', '[]'); | ||
|
|
||
| $ecsHost = parse_url(self::ECS_ENDPOINT)['host'] ?? ''; |
There was a problem hiding this comment.
no need to parse the entier url, when you are looking only for the host
| $ecsHost = parse_url(self::ECS_ENDPOINT)['host'] ?? ''; | |
| $ecsHost = parse_url(self::ECS_ENDPOINT, \PHP_URL_HOST) ?: ''; |
kevinrudde
force-pushed
the
add-support-for-pod-identity
branch
from
18c06e5 to
8d07181
Compare
|
Hey @jderusse 👋 |
|
thank you @kevinrudde |
2 participants
Added support for EKS Pod Identity, see https://docs.aws.amazon.com/eks/latest/userguide/pod-identities.html
It is similar to the
ContainerProvider, but uses the injected environment variables from the Pod Identity Agent.